Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added: [M3-6989] - aria-describedby to TextField with helper text #11351

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Dec 2, 2024

Description 📝

Tiny PR to add better accessibility to the Textfield via its helperText prop.

On screen, it is trivial for a user to see a helper test associated with a field, but this is not necessarily true for someone using a screen reader, since the helper text is a plain paragraph floating under the input, and isn't associated with its relevant interactive element.

Changes 🔄

  • Describe the textfield by its helper text when disabled
  • Cleanup textfield input ID logic (see self review)

Preview 📷

Screenshot 2024-12-02 at 16 39 25

How to test 🧪

Prerequisites

Verification steps

  • Check markup to confirm Textfield & HelperText relationship
  • Optional: confirm behavior with VoiceOver util

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@abailly-akamai abailly-akamai self-assigned this Dec 2, 2024
@abailly-akamai abailly-akamai added the Accessibility Contains accessibility improvements or changes label Dec 2, 2024
@@ -113,7 +113,8 @@ interface InputToolTipProps {
tooltipWidth?: number;
}

interface TextFieldPropsOverrides extends StandardTextFieldProps {
interface TextFieldPropsOverrides
extends Omit<StandardTextFieldProps, 'label'> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps TS in general not having to compare overridden types. While in this case this is just a string and this likely has no impact, it's just good practice.

(label
? convertToKebabCase(label)
: // label could still be an empty string
fallbackId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always making sure we have an ID

@abailly-akamai abailly-akamai changed the title change: [M3-6989] Add aria-describedby to TextField with helper text added: [M3-6989] Add aria-describedby to TextField with helper text Dec 3, 2024
@abailly-akamai abailly-akamai changed the title added: [M3-6989] Add aria-describedby to TextField with helper text added: [M3-6989] - aria-describedby to TextField with helper text Dec 3, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review December 3, 2024 02:50
@abailly-akamai abailly-akamai requested a review from a team as a code owner December 3, 2024 02:50
@abailly-akamai abailly-akamai requested review from dwiley-akamai and hkhalil-akamai and removed request for a team December 3, 2024 02:50
Copy link

github-actions bot commented Dec 3, 2024

Coverage Report:
Base Coverage: 86.85%
Current Coverage: 86.85%

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make the same change when displaying error messages. VoiceOver currently isn't able to pick them up:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hkhalil-akamai That's fair, this one has a role="alert" attribute which is very much meant for this type of behavior (aka, a validation error injected into the DOM) but could indeed use more context.

see commit 95a8d3e which follows this recommended pattern

@abailly-akamai abailly-akamai force-pushed the M3-6989-helper-text-accessibility branch from 95a8d3e to fd53887 Compare December 4, 2024 15:53
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ✅
Confirmed TextField <--> HelperText relationship ✅

Screenshot 2024-12-04 at 11 27 40 AM

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 3 failing tests on test run #8 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
3 Failing462 Passing2 Skipped107m 58s

Details

Failing Tests
SpecTest
rebuild-linode.spec.tsrebuild linode » rebuilds a linode from Image
rebuild-linode.spec.tsrebuild linode » rebuilds a linode from Community StackScript
rebuild-linode.spec.tsrebuild linode » rebuilds a linode from Account StackScript

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/rebuild-linode.spec.ts,cypress/e2e/core/linodes/rebuild-linode.spec.ts,cypress/e2e/core/linodes/rebuild-linode.spec.ts"

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing feedback!

@abailly-akamai abailly-akamai merged commit 04f241d into linode:develop Dec 6, 2024
22 of 23 checks passed
Copy link

cypress bot commented Dec 6, 2024

Cloud Manager E2E    Run #6931

Run Properties:  status check passed Passed #6931  •  git commit 04f241dc8d: added: [M3-6989] - `aria-describedby` to TextField with helper text (#11351)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6931
Run duration 28m 27s
Commit git commit 04f241dc8d: added: [M3-6989] - `aria-describedby` to TextField with helper text (#11351)
Committer Alban Bailly
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 466
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Contains accessibility improvements or changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants